Skip to content

Conversation

@George-Guryev-flxcmp
Copy link
Contributor

@George-Guryev-flxcmp George-Guryev-flxcmp commented Nov 25, 2025

This PR adds feature for automatic extrusion of structures located next to the simulation boundaries into PML/Absorber. This is facilitated by additional field enable_extrusion in class AbsorberSpec.

Greptile Overview

Greptile Summary

This PR adds an API field enable_extrusion to the AbsorberSpec class, allowing users to enable automatic extrusion of structures into PML/Absorber boundary regions. The field is properly documented and tested.

Key Changes:

  • Added enable_extrusion boolean field to AbsorberSpec class (inherited by PML, StablePML, and Absorber)
  • Field defaults to False to maintain backward compatibility
  • Added comprehensive test coverage verifying the field works only on absorber types
  • Updated all simulation schemas to reflect the new field
  • Added clear changelog entry

Important Notes:

  • This appears to be an API-only addition; the actual implementation logic that uses this field is not present in this PR
  • The field is stored but not consumed by any simulation or validation logic yet
  • Similar to the extrude_structures field in WavePort, this likely prepares the groundwork for future implementation

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk as it only adds an API field without implementation logic
  • Score of 4 reflects that this is a clean API addition with proper documentation and tests, but the field is not yet used anywhere in the codebase. Minor style improvements suggested for test assertions. The change is backward compatible and follows established patterns.
  • No files require special attention - all changes are straightforward API additions

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/boundary.py 5/5 Added enable_extrusion boolean field to AbsorberSpec class with proper default value and documentation
tests/test_components/test_boundaries.py 4/5 Added test for enable_extrusion field checking default values and exclusivity to absorber types; uses direct equality instead of is for boolean checks
CHANGELOG.md 5/5 Added clear user-facing changelog entry documenting the new enable_extrusion field in AbsorberSpec

Sequence Diagram

sequenceDiagram
    participant User
    participant Simulation
    participant BoundarySpec
    participant AbsorberSpec
    participant PML/Absorber
    
    User->>Simulation: Create simulation with boundary conditions
    User->>PML/Absorber: PML(enable_extrusion=True)
    PML/Absorber->>AbsorberSpec: Initialize with enable_extrusion field
    AbsorberSpec->>AbsorberSpec: Store enable_extrusion=True
    AbsorberSpec-->>PML/Absorber: Return configured boundary
    PML/Absorber-->>Simulation: Add to boundary specification
    Simulation->>BoundarySpec: Configure x/y/z boundaries
    
    Note over Simulation,BoundarySpec: Field is stored but not yet used in implementation
    Note over User,PML/Absorber: API surface prepared for future extrusion logic
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Nov 25, 2025

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

  • tidy3d/components/boundary.py (100%)

Summary

  • Total: 2 lines
  • Missing: 0 lines
  • Coverage: 100%

@daquinteroflex
Copy link
Collaborator

Are we sure this is not required for rc3? It's a schema change that will be blocked till 2.11. @George-Guryev-flxcmp @dbochkov-flexcompute @weiliangjin2021

@weiliangjin2021
Copy link
Collaborator

Are we sure this is not required for rc3? It's a schema change that will be blocked till 2.11. @George-Guryev-flxcmp @dbochkov-flexcompute @weiliangjin2021

If so, let's try to get it in for rc3. When is the deadline for merging PRs for rc3?

@daquinteroflex
Copy link
Collaborator

If so, let's try to get it in for rc3. When is the deadline for merging PRs for rc3?

Can you get this in for tomorrow?

@George-Guryev-flxcmp
Copy link
Contributor Author

If so, let's try to get it in for rc3. When is the deadline for merging PRs for rc3?

Can you get this in for tomorrow?

Sure thing.

@daquinteroflex daquinteroflex added rc3 3rd pre-release 2.10 and removed rc3 3rd pre-release 2.10 labels Nov 26, 2025
@George-Guryev-flxcmp George-Guryev-flxcmp force-pushed the george/extrusion_to_pml branch 5 times, most recently from 98c56f5 to a0cba1d Compare December 2, 2025 06:39
@George-Guryev-flxcmp George-Guryev-flxcmp force-pushed the george/extrusion_to_pml branch 2 times, most recently from c9522d4 to 222c6fb Compare December 9, 2025 02:02
Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also need to override _structures_not_at_edges

@pd.validator("structures", always=True)
@skip_if_fields_missing(["size", "center"])
def _structures_not_at_edges(cls, val, values):
"""Warn if any structures lie at the simulation boundaries."""
if val is None:
return val
sim_box = Box(size=values.get("size"), center=values.get("center"))
sim_bound_min, sim_bound_max = sim_box.bounds
sim_bounds = list(sim_bound_min) + list(sim_bound_max)
with log as consolidated_logger:
for istruct, structure in enumerate(val):
struct_bound_min, struct_bound_max = structure.geometry.bounds
struct_bounds = list(struct_bound_min) + list(struct_bound_max)
for sim_val, struct_val in zip(sim_bounds, struct_bounds):
if anp.isclose(sim_val, struct_val):
consolidated_logger.warning(
f"Structure at 'structures[{istruct}]' has bounds that extend exactly "
"to simulation edges. This can cause unexpected behavior. "
"If intending to extend the structure to infinity along one dimension, "
"use td.inf as a size variable instead to make this explicit.",
custom_loc=["structures", istruct],
)
continue
return val
with a modified function for Simulation

Comment on lines +3481 to +3484
if boundary[0].extrude_structures:
warn_extruded(structure, istruct, axis + "-min")
else:
warn(structure, istruct, axis + "-min")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if a structure is less than half wavelength away from pml, but still not close enough to be extruded?

False,
title="Enable structure extrusion to PML",
description="Automatically extrude structures into the absorbing region (e.g., PML or adiabatic absorber)."
"Any structure located within `CLIP_OFFSET` of a simulation boundary will be extended through the full thickness of the PML/absorber."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably better to change within CLIP_OFFSET of to within {CLIP_OFFSET} cells of

custom_loc=["structures", istruct],
)

def warn_extruded(structure, istruct, side) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider extending the original warn with additional parameter, instead of a new mostly duplicating function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants